-
Notifications
You must be signed in to change notification settings - Fork 6
feat: Enable true tokenless for bundler plugins #189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files
☔ View full report in Codecov by Sentry. |
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found.
📢 Thoughts on this report? Let us know! |
Bundle ReportChanges will decrease total bundle size by 3.54kB (-0.06%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
Bundle ReportChanges will decrease total bundle size by 764 bytes (-0.03%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
suejung-sentry
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example/tokenless - what setup do we do on the repo/github actions/other to make that go through successfully? Said another way - is there somewhere I can read to learn more on how to get "true tokenless" configured? Mostly to make sure our tests in here will continue to cover the scenario
| codecovVitePlugin({ | ||
| enableBundleAnalysis: true, | ||
| bundleName: "@codecov/example-tokenless-app", | ||
| uploadToken: process.env.TOKENLESS_UPLOAD_TOKEN, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we still supporting the "tokenlessForForks"? If so, we should add a separate example for that scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we are keeping tokenlessForForks live. good call - I agree there should be an example/test to show that tokenlessForForks are still valid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would platform side of things complain if we set the branch to codecov:<branch-name> for the example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or does it have to be a username?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be hard to fudge the :
to get the branch:
first - for BA, shelter gets the commit_id from the commit field in the body of the request. The commit and repo have to exist in firestore - I'm thinking they will. If they exist, shelter uses the branch from the commit.
if shelter can't get the commit (if you found a way to break the above method), it will use the branch field in the body of the request.
and then we have an additional check just for BA uploads, that the branch (most likely from the commit) has to match the branch in the body of the request.
This might explain why the tokenless example doesn't work on shelter
packages/bundler-plugin-core/src/utils/__tests__/getPreSignedURL.test.ts
Show resolved
Hide resolved
|
I learned a lot from @suejung-sentry about examples and how they work. I didn't realize they were using real data from the pr. This means an example for True Tokenless will never succeed to upload, because the Codecov Org will never allow tokenless uploads (it's determined by a field on the OwnerOrg). So if you want an example of a passing True Tokenless upload, you need to run it on a different OwnerOrg, which becomes more of an integration test than an example. We also learned that the current tokenless example has been failing since the switch to After this pr is merged, BA will work for True Tokenless, and will continue to not work for tokenless v2 |
suejung-sentry
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add integration tests for the tokenless v3 (true tokenless) scenario and also a separate example org demonstrating true tokenless with a separate ticket
|
Yea if we're unable to provide a true tokenless example that also uploads data, I'm alright still having something in there, that people can look at. |
Description
Tokenless v2 has requirements for branch format, True Tokenless is allowed on any branch.
There are a few places in code (across several projects) where a request without a token is rejected if it doesn't have the fork format (branch contains
:). I need to remove those blocks and allow those requests through. There are auth methods for True Tokenless inshelterandapi, the validity of the tokenless uploads will be checked there.true tokenless auth check for BA on api: codecov/codecov-api#882
I couldn't find instructions for local dev, so I will need help with lint and tests, made my best guess for both 🙃
codecov/engineering-team#2877